Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow subscribing to reflector store updates #1426

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

nightkr
Copy link
Member

@nightkr nightkr commented Mar 13, 2024

Motivation

This is largely a "shower thoughts" take on #1189 and the discussions from the last office hours.

It still fails controller::tests::applier_must_not_deadlock_if_reschedule_buffer_fills and introduces some type safety regressions that I'm not quite sure how to fix.

Solution

It introduces a new (internal) Broadcaster channel, which has the backpressure and broadcasting properties that we need (async-channel is inappropriate after all since it only sends each message to one receiver).

It then adds an interface to reflector::store::Writer for subscribing to changes.

This all leads to a fair amount of changes to reflector and its downstreams to accomodate for Writer::apply_watcher_event now being async (so that it can send out the appropriate events).

Signed-off-by: Natalie Klestrup Röijezon <[email protected]>
Signed-off-by: Natalie Klestrup Röijezon <[email protected]>
Signed-off-by: Natalie Klestrup Röijezon <[email protected]>
Signed-off-by: Natalie Klestrup Röijezon <[email protected]>
Signed-off-by: Natalie Klestrup Röijezon <[email protected]>
@mateiidavid
Copy link
Contributor

hey, just linking this here since I've been working on the same thing

main...mateiidavid:kube:matei/arc-watcher

I too switched to async-broadcast after our discussion since it's better suited for what we need to do.

@nightkr
Copy link
Member Author

nightkr commented Mar 13, 2024

Ah, hadn't seen that one. However, that also looks like it prefers to drop messages rather than apply backpressure (https://docs.rs/async-broadcast/latest/async_broadcast/enum.RecvError.html#variant.Overflowed).

@mateiidavid
Copy link
Contributor

I think overflow is disabled by default. It can also be disabled manually (see https://docs.rs/async-broadcast/latest/async_broadcast/struct.Receiver.html#method.overflow).

The Send future does have annoying lifetimes so what I ended up doing is using try_recv instead of trying to build a state machine to resume after backpressure was applied. The function returns a RecvError that is either Error::Full or Error::Closed (slightly paraphrasing the types).

I actually wanted to send that over to you for a quick sanity check but I guess now's as good a time as any 😂

@nightkr
Copy link
Member Author

nightkr commented Mar 13, 2024

I think overflow is disabled by default. It can also be disabled manually (see https://docs.rs/async-broadcast/latest/async_broadcast/struct.Receiver.html#method.overflow).

Ahh: https://docs.rs/async-broadcast/latest/src/async_broadcast/lib.rs.html#156, gotcha, looks reasonable.

The Send future does have annoying lifetimes so what I ended up doing is using try_recv instead of trying to build a state machine to resume after backpressure was applied. The function returns a RecvError that is either Error::Full or Error::Closed (slightly paraphrasing the types).

I assume you mean try_broadcast? Sadly that won't work, since it won't register with the waker (so that it knows when it's time to retry). That'll deadlock until the next time that the future is polled for some other reason.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants